Rename prohibitAllSharing to containsRestrictedData - #234
Conversation
github-actions
Bot
added
workshop/frontend
c04d5e3 to
aa71300
Compare
fd04b0d to
42aefbf
Compare
cf3449d to
f633a1a
Compare
5f1ff10 to
e4e661c
Compare
f633a1a to
bc6f07a
Compare
Preview:
|
413e955 to
b2c3e58
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cd22d16 to
b7c9372
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
41ae6a0 to
740374a
Compare
|
|
||
| const isOwner = !metadata.owner | ||
| const sharingProhibited = metadata.sharingProhibited === true | ||
| const containsRestrictedData = metadata.containsRestrictedData === true |
There was a problem hiding this comment.
🟡 Kernel and UI changes are shipped in the same commits, against the repository's review rule
The change edits the kernel and the browser UI together in single commits (packages/workshop-frontend/src/ShareModal.tsx:374 alongside packages/workshop-backend/src/overseer.ts), so the kernel cannot be reviewed apart from the UI as the repository requires.
Impact: Reviewers cannot review the security-critical kernel diff independently, which the project's review policy mandates.
Rule and commit layout
REVIEW.md ("High-scrutiny areas"): "A large kernel change should be split by concern into separate PRs, and at minimum grouped into commits that let workshop-backend/workshop-shared be reviewed apart from UI. Flag PRs that bundle both."
Commits 6b778a1 and 1991473 each touch packages/workshop-backend/src/overseer.ts, packages/workshop-shared/src/api.ts, packages/workshop-shared/src/gatekeeper.ts and packages/workshop-frontend/src/ShareModal.tsx in one commit.
Prompt for agents
Per REVIEW.md, kernel changes (packages/workshop-backend, packages/workshop-shared) must be reviewable apart from UI changes. Restructure the history so the ShareModal.tsx (workshop-frontend) edits live in their own commit(s) separate from the overseer.ts/api.ts/gatekeeper.ts changes, or split the frontend change into its own PR.
Was this helpful? React with 👍 or 👎 to provide feedback.
| vendorId !== null && this.#inScopeGatekeepers("use").some(gk => gk.id === gatekeeperId); | ||
|
|
||
| for (let collaborator of collaborators) { | ||
| // A verifiable gatekeeper outside a "use" collaborator's scope is one the UI can't invoke | ||
| // and ensureObserver can't cover; only the unverifiable case above blocks regardless of | ||
| // role. An absent role means "build" (see CollaboratorInfo), which fails safe here. | ||
| if (vendorId && (collaborator.role ?? "build") === "use" && !inUseScope) continue; | ||
| let observer = vendorId ? this.storage.observers.get(collaborator.profile.id) : undefined; | ||
| if (!observer || !(gatekeeperId in observer.accountChoices)) { | ||
| throw new Error( | ||
| "This observation was blocked because it contains sensitive data, but this " + | ||
| `workspace is shared with ${collaborator.profile.name} (${collaborator.profile.id}), ` + | ||
| "who has not been verified to have access to that data. They must re-open the " + | ||
| "workspace (which verifies their access) or be removed from it before this data " + | ||
| "can be read."); | ||
| } |
There was a problem hiding this comment.
🟨 A "use" collaborator can now open a workspace holding restricted data without being verified against the connection that read it
Removing the wholesale block in open() (packages/workshop-backend/src/overseer.ts:6751-6756) means a non-owner may now open a workspace that has observed containsRestrictedData, and verification is only run for gatekeepers in their role's scope. For a use collaborator, #inScopeGatekeepers("use") (packages/workshop-backend/src/overseer.ts:6091-6112) only includes connections bound by a non-pending gadget, so a connection that read restricted data through the agent's chat bindings only (never bound to a gadget) is never verified against them, and the observation-time coverage guard explicitly skips them for that connection (packages/workshop-backend/src/overseer.ts:3002). If the agent persists that restricted data into gadget code/storage, the use collaborator renders it through the gadget UI with no access check at all — previously impossible, since a latched workspace refused every non-owner open.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| throw new Error( | ||
| "This observation was blocked because it contains sensitive data, but this " + | ||
| `workspace is shared with ${collaborator.profile.name}, ` + | ||
| "who has not been verified to have access to that data. They must re-open the " + | ||
| "workspace (which verifies their access) or be removed from it before this data " + | ||
| "can be read."); | ||
| } |
There was a problem hiding this comment.
🟨 Blocked sensitive read names a collaborator to sandboxed gadget and agent output
The coverage-guard error embeds collaborator.profile.name (packages/workshop-backend/src/overseer.ts:3020) and, as its own comment notes, this message is surfaced to sandboxed gadget code and agent output — an audience that cannot otherwise enumerate collaborators. The comment claims the profile id is omitted because it is "often an email address", but profile.name is the user-supplied display name and, for password signups, is set to the username/email (see createAccount(username, username, ...) usage), so the identifier the guard intends to withhold can still be exposed.
Was this helpful? React with 👍 or 👎 to provide feedback.
108a748 to
9d7c2fe
Compare
9d7c2fe to
d3095c6
Compare
Rename prohibitAllSharing to containsRestrictedData: the flag now means "this observation carries data whose sharing is governed by observer verification" rather than a wholesale ban on sharing the workspace. The legacy flag is still read on persisted records and converted on load. Instead of refusing to share a flagged workspace outright, sharing is admitted when every collaborator is verified as an observer of the producing gatekeeper: - open() verifies each collaborator against every gatekeeper in their role's verification scope (ensureObserver), and the coverage guard (#assertSensitiveObservationCoverage) blocks a sensitive observation naming any current collaborator not yet verified against its producer. Coverage is held to each collaborator's role scope, since a "use" collaborator can never be verified against a gatekeeper no gadget binds. - Share-key redemption writes a *pending* edge that grants no interim authority: the redeeming open() verifies the recipient at the role the edge would grant, confirms only on success (capped at the verified role, denied if the connection/binding topology changed mid-verification, merged if a concurrent redemption landed first), and rolls the edge back on refusal or a null effective role. - Removing the producing connection no longer lifts the restriction for existing collaborators, and assertNewSharingAllowed refuses new grants inside the grant write's synchronous block. - The coverage-guard error reaches sandboxed gadget/agent output, so it names the collaborator but omits their profile id (the full email on OAuth/CF Access deployments). Three hardening rounds are folded in: - A failed live re-check scrubs the failed gatekeeper from the collaborator's persisted observer record synchronously with the failure determination (covering both failure sites and cancel-after-reprompt), and the terminal catch best-effort de-registers invalidated gatekeepers alongside newly-added ones (removeObserver is idempotent). Without this, a collaborator whose provider-side access was revoked kept admitting the producer's restricted observations to their still-live older session. The scrub is scoped to the failed gatekeeper; a repaired pass re-persists full coverage. Fail-closed by design: an operational failure (outage, expired credential) scrubs the same way, blocking that producer's restricted reads until the collaborator re-opens successfully. - confirmShareKeyRedemption re-asserts the redemption policy via the same optional assertGrantAllowed callback the other grant-writing mutators use, in the granting write's synchronous block: redemption is two-phase, and a restricted-data producer removed between the pending write and the confirm -- invisible to the topology fingerprint, since an unverifiable legacy producer's remove() skips the share-link guard -- must still refuse the grant. An already-confirmed edge skips it, matching redeemShareKey. - The accepted pending-edge re-add wart is documented precisely: a racing removal necessarily aimed at a previously confirmed edge (pending-only recipients are invisible to listCollaborators), the re-add carries no incremental authority (the recipient holds the live, manually re-redeemable link), and revoking the link is the durable exclusion. The accepted residuals (formerly-bound and never-bound producers outside "use" scope) are documented in docs/observers.md edge case 4 and docs/sharing.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e scope New sensitive-observations and observer-role-scope suites, with the harness, RPC client, and gatekeeper-test fixture support they need (the fixture gains a configurable observer hook and sensitive reads). The fixture's verify outcome can also target a single bound resource (a resource-specific key with a label-wide fallback), which the coverage-scrub test uses to prove the scrub is per-producer: after a failed re-verification, exactly the refused producer's restricted reads are blocked -- the sibling producer's keep flowing -- until a repaired re-open re-persists coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Update gatekeeper-google to the renamed flag, and revise the gatekeeper-mcp README and write-gatekeeper skill guidance to describe observer-verified sharing rather than a wholesale sharing prohibition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The modal no longer refuses to share a workspace with restricted data;
it explains that collaborators must be verified against the producing
connection at their next open. The share key is retained across
failures and reloads (sessionStorage tier) so a recipient who is
denied verification can retry, and is discarded at the first
successful open so a later removal isn't undone by an automatic
re-redemption.
The sessionStorage tier lives in retainedShareKeys.ts (so useAuth does
not import the workspace hook) and is bound to the user who captured
the key, so it cannot cross users in a shared tab:
- Entries are identity-stamped (JSON {key, userId}) with the capturing
session's whoami, resolved from the same stub the open is issued on.
The retained-read path honors an entry only when the current
session's identity matches; a definite mismatch sweeps it, a
transport failure leaves it but does not attach the key. The common
keyless open stays fully pipelined -- identity is only resolved when
a fragment key is captured (async, gated against the success-discard
racing it) or a stored entry exists (rare: only after a reload
mid-retry). Malformed or unstamped entries read as absent.
- logout() sweeps the whole retention prefix, before the CF Access
navigate-away.
The in-memory ref tier needs no stamp: it is bounded by the editor's
lifetime (logout unmounts it; CF Access logout navigates away).
Residual: a reload before the identity stamp lands loses retention,
recovered by re-clicking the invite link.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a2c035d to
cbfaf39
Compare
d3095c6 to
d120690
Compare
1 participant
Old model: prohibitAllSharing blocked all sharing the moment sensitive data was observed. No one other than the owner could access a latched workspace.
New model: containsRestrictedData allows sharing but requires per-collaborator observer verification against each gatekeeper that read sensitive data. The workspace still goes into a restricted mode (no actions against gatekeepers, no web fetches), but collaborators can open it if they pass verification.
WIP